Xen devel - Apr 2006 - [PATCH 0/5] pciback: new configuration space fields, permissive flag

The following patches add some new capabilities to the PCI Backend''s
virtual configuration space handler (such as support for the tg3 network
card and for the Vital Product Data and Power Management structures on
the capability list). These patches must be applied together (I''ve
tried
to divide them up into logical groups of functionality for easier
review, but there is a bit of overlap).

These patches also contain some general formatting fixes and renaming of
a few functions to clarify their purpose in light of the new code.




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 11 Apr 2006, at 19:31, Ryan wrote:
> The following patches add some new capabilities to the PCI
Backend''s
> virtual configuration space handler (such as support for the tg3 
> network
> card and for the Vital Product Data and Power Management structures on
> the capability list). These patches must be applied together (I''ve
> tried
> to divide them up into logical groups of functionality for easier
> review, but there is a bit of overlap).
>
> These patches also contain some general formatting fixes and renaming 
> of
> a few functions to clarify their purpose in light of the new code.
It seems to me that this splits policy decisions on permissiveness of 
access to a particular PCI device between user space and kernel. 
Specifically, to make good automatic use of the per-device permissive 
flag we will need a mapping in user space from device ids to correct 
setting of the flag. If you leave it manually to the user, you know 
which way it will always be set. :-) At the same time, in the kernel we 
have a mapping from device ids to filtering rules, which are just 
another facet of filtering policy.

I think it would be much neater to implement only the enforcement 
mechanisms in the kernel driver, and to move all the rules about which 
registers may be accessed for which device types out into user space. 
Then, when binding a PCI device to pciback we would also squirt the 
filtering rules into the kernel. This seems to me preferable for a 
number of reasons:
  1. We don''t end up with a scaling mess of extra C source files for 
every new device we come across.
  2. Maintain the rules in an easier to edit format in text files
  3. One place we maintain mappings from device ids -> filtering policies

The main downside is the extra work to push the rules into the kernel 
and do whatever parsing is required. It does feel like the right way to 
go, though.

  -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2006-04-13 at 10:06 +0100, Keir Fraser wrote:
> 
> It seems to me that this splits policy decisions on permissiveness of 
> access to a particular PCI device between user space and kernel. 
> Specifically, to make good automatic use of the per-device permissive 
> flag we will need a mapping in user space from device ids to correct 
> setting of the flag. If you leave it manually to the user, you know 
> which way it will always be set. :-) At the same time, in the kernel we 
> have a mapping from device ids to filtering rules, which are just 
> another facet of filtering policy.
> 
> I think it would be much neater to implement only the enforcement 
> mechanisms in the kernel driver, and to move all the rules about which 
> registers may be accessed for which device types out into user space. 
> Then, when binding a PCI device to pciback we would also squirt the 
> filtering rules into the kernel. This seems to me preferable for a 
> number of reasons:
>   1. We don''t end up with a scaling mess of extra C source files
for
> every new device we come across.
>   2. Maintain the rules in an easier to edit format in text files
>   3. One place we maintain mappings from device ids -> filtering
policies
> 
> The main downside is the extra work to push the rules into the kernel 
> and do whatever parsing is required. It does feel like the right way to 
> go, though.
> 
I''d thought about this option and perhaps it is a good option for
handling device specific configuration fields. Another downside is that
you lose the ability to do any custom handling of those fields (at least
not without a lot of complicated back-and-forth between the kernel and
some user-space pci daemon). For example, I don''t think it would be
possible to do in user-space what we do now for intercepting calls to
registers like PCI_COMMAND or the new PCI power management stuff I added
with these patches (which make calls to core pci functions in the
kernel). All a user-space policy could then contain is whether a given
field is writable or not (and maybe a mask of exactly which bits are
writable). However, that''s probably enough for most devices (although I
can''t say for certain having only had a cursory look at the tg3 driver
and no others). I think the header fields and structures on the
capability list will need to remain in kernel-space.

I also wasn''t sure when was a good time to inject these rules into the
pciback driver. Right now, the configuration fields get added to a card
when it is probed. I suppose I could generate some kind of hotplug event
to trigger that. Or perhaps some kind of lazy initialization that takes
place through xend the first time that a PCI device is given to a driver
domain.

I see your point about the permissive flag. While I think this method is
better than the global flag, I think you''re right: we can improve it
more. I could also do a lazy initialization on the permissive flag
(perhaps moving it from a sysfs attribute to xend matching a device id
and setting it in XenStore). Is that more along the lines of what you
were thinking?

Do you have any other comments/concerns about adding the capability list
handlers (like power management) or the bottom-half handler for pci
operations from the frontend?

Ryan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 13 Apr 2006, at 14:17, Ryan wrote:
> All a user-space policy could then contain is whether a given
> field is writable or not (and maybe a mask of exactly which bits are
> writable). However, that''s probably enough for most devices
(although I
> can''t say for certain having only had a cursory look at the tg3
driver
> and no others). I think the header fields and structures on the
> capability list will need to remain in kernel-space.
Fair enough. I expect that the fields that need extra processing in 
pciback will be generic things like power management etc? So really 
device-specific things (tg3, ide, etc.) simply need at worst bit-wise  
filtering?
> I also wasn''t sure when was a good time to inject these rules into
the
> pciback driver. Right now, the configuration fields get added to a card
> when it is probed. I suppose I could generate some kind of hotplug 
> event
> to trigger that. Or perhaps some kind of lazy initialization that takes
> place through xend the first time that a PCI device is given to a 
> driver
> domain.
Doing it lazily makes sense. Management tools must actively bind a PCI 
device to a new domain, and it makes sense to push down device-specific 
policy at the same time.
> I see your point about the permissive flag. While I think this method 
> is
> better than the global flag, I think you''re right: we can improve
it
> more. I could also do a lazy initialization on the permissive flag
> (perhaps moving it from a sysfs attribute to xend matching a device id
> and setting it in XenStore). Is that more along the lines of what you
> were thinking?
Yes, although you could potentially just leave the switch in sysfs. Or 
do you want to keep it away from direct user interference?

I wouldn''t hard-code the permissive device ids in xend. Even something 
really simple like a single commented text file containing lists of 
device ids and the appropriate policy (e.g., permissive, or a list of 
extra config-space fields that can be read and/or written) would be 
better imo.
> Do you have any other comments/concerns about adding the capability 
> list
> handlers (like power management) or the bottom-half handler for pci
> operations from the frontend?
That all made sense, although I can''t say I read the patches in huge 
detail. It''s mainly the wish to avoid hardcoding per-device policy in 
the kernel that got me worried.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2006-04-13 at 14:45 +0100, Keir Fraser wrote:
> On 13 Apr 2006, at 14:17, Ryan wrote:
> 
> > All a user-space policy could then contain is whether a given
> > field is writable or not (and maybe a mask of exactly which bits are
> > writable). However, that''s probably enough for most devices
(although I
> > can''t say for certain having only had a cursory look at the
tg3 driver
> > and no others). I think the header fields and structures on the
> > capability list will need to remain in kernel-space.
> 
> Fair enough. I expect that the fields that need extra processing in 
> pciback will be generic things like power management etc? So really 
> device-specific things (tg3, ide, etc.) simply need at worst bit-wise  
> filtering?
That''s probably correct. The generic things like power management and
the header registers should be the only things that need to call the
kernel''s pci functions (on a side note, I don''t know how
useful the
power management stuff is right now, but it was intended as more of an
example for how other capability list structures might be handled; in
particular, I thought that someday when MSI is supported in Xen, perhaps
the driver domain''s interface for setting up MSI for a device could
happen through these virtual configuration space fields (although I''m
not familiar enough with MSI to know if that is feasible or not)). A
quick glance through the kernel at other drivers that use
device-specific fields leads me to believe that this will be enough (I
suppose it would be better to wait and revisit this if someone ever
finds a device/driver that demonstrates that it''s not enough).
> I wouldn''t hard-code the permissive device ids in xend. Even
something
> really simple like a single commented text file containing lists of 
> device ids and the appropriate policy (e.g., permissive, or a list of 
> extra config-space fields that can be read and/or written) would be 
> better imo.
Is it appropriate for xend to read/write a configuration file? xend''s
clients (xm) are the ones that do the processing of config. files now
and I wasn''t sure if it was acceptable to have xend reach into a
configuration file (say /etc/xen/pci_permissive_ids.conf). I believe it
must happen on xend, but if it''s not hard-coded in, what would be the
appropriate place to read the config file from?
> 
> > Do you have any other comments/concerns about adding the capability 
> > list
> > handlers (like power management) or the bottom-half handler for pci
> > operations from the frontend?
> 
> That all made sense, although I can''t say I read the patches in
huge
> detail. It''s mainly the wish to avoid hardcoding per-device policy
in
> the kernel that got me worried.
> 
I understand and agree. I''ll fix them and resubmit.

Ryan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 13 Apr 2006, at 16:40, Ryan wrote:
> , I thought that someday when MSI is supported in Xen, perhaps
> the driver domain''s interface for setting up MSI for a device
could
> happen through these virtual configuration space fields (although
I''m
> not familiar enough with MSI to know if that is feasible or not)).
For the specific example of MSI this will not happen --- MSI will be 
configured (or not) at bind time by domain 0. We will not run the MSI 
driver at all outside domain 0 (I need to work out how to arrange for 
the MSI driver to not activate outside domain0 though).
> Is it appropriate for xend to read/write a configuration file?
xend''s
> clients (xm) are the ones that do the processing of config. files now
> and I wasn''t sure if it was acceptable to have xend reach into a
> configuration file (say /etc/xen/pci_permissive_ids.conf). I believe it
> must happen on xend, but if it''s not hard-coded in, what would be
the
> appropriate place to read the config file from?
xend reads /etc/xen/xend-config.sxp already. I think placing a file in 
there would be a good idea.

What do you think about format? File per device type? One file for 
everything, including devices that are not permissive but have extra 
filtering rules? One file for permissive ids, and one for devices with 
extra filtering rules?...

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2006-04-13 at 17:26 +0100, Keir Fraser wrote:
> xend reads /etc/xen/xend-config.sxp already. I think placing a file in 
> there would be a good idea.
> 
> What do you think about format? File per device type? One file for 
> everything, including devices that are not permissive but have extra 
> filtering rules? One file for permissive ids, and one for devices with 
> extra filtering rules?...
> 
I still need to think about it a bit to see what will work best. My
initial thought was to use s-expressions as the file format and to have
one file for permissive ids and one for the filtering rules (since we
only have one device to support for now). But I''ll think about it some
more. I''m not sure what would be a good interface to get this data from
xend to PCI Backend yet...

I''d like to avoid making the PCI Backend dependent on being in the same
domain as xend as it''d be nice to have the option of placing it in its
own driver domain someday. Reading the device''s resources through sysfs
and having the permissive flag only in sysfs got away from this goal.
Perhaps having a means to set the permissive flag through XenStore/xend
will correct this issue. Maybe the filtering rules should also get
pushed into the backend through XenStore somehow. I''ll need to think
about this some more.

Ryan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel