Re: [PATCH 2/3] vfio: vfio-pci device assignment driver

[Avi Kivity <avi@redhat.com>]

On 08/14/2012 08:23 PM, Alex Williamson wrote:
> 
>> Unrelated nit: memcmp() doesn't return a boolean or a count, so
>> !memcmp() is really unintuitive, at least to me.
> 
> I figure we're all pretty used to it growing up on !strcmp though.

I hate that one too.

>> > +
>> > +/* XXX This should move to msi.c */
>> 
>> Well?
> 
> Just marking a todo item.  I'll change it formally to TODO.  I think
> there are a few interfaces to msi.c that probably needs some rethinking
> for device assignment.  When they're small like this it seems easier to
> have the user in tree first.

I prefer them in the right place but I don't insist.

>> > +
>> > +    if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) !=
>> > +                 (section->offset_within_region & ~TARGET_PAGE_MASK))) {
>> > +        error_report("%s received unaligned region\n", __func__);
>> 
>> Is it really an error?  I think you can just add the condition to
>> skipped_section.
> 
> I had left this in as paranoia for myself that I wanted to see if this
> actually happens.  I want to assume that our TARGET_PAGE_ALIGNED
> offset_within_address_space results in an aligned ram pointer.  If one
> is aligned different from the other we're kinda screwed trying to map it
> into the iommu.  So far I haven't seen it.  Thanks for the feedback,

We could have a sub-page RAM region (perhaps inserted as a mapped BAR
from some emulated device, or from vfio if/when it grows that capability).

But you're right, it really is an error, we can't just ignore it.  So
the current code is right.

-- 
error compiling committee.c: too many arguments to function