Correct way to enable BusMaster with VFIO?

Correct way to enable BusMaster with VFIO?

[Wei Hu]

Hi Alex,

With your change "Release devices with BusMaster disabled", I've found
that my VFIO device driver is no longer receiving MSI interrupts.
After reviewing the code I think it makes sense. But I had two
questions below while debugging my issue.

1.  If I had set the bus master bit in the command register by hand
before opening the vfio device, the kernel would actually leave
BusMaster enabled. This seems to contradict the call to
pci_clear_master() from vfio_pci_enable(). What's going on here, is
something else enabling BusMaster?

2. What's the recommended way to enable BusMaster with your change
now? Should my driver map the config space region and set the
BusMaster bit? Or should I have a separate command to enable the bit
before opening the device?

Thank you,
Wei

Re: Correct way to enable BusMaster with VFIO?

[Alex Williamson]

Hi Wei,

On Tue, 2015-04-07 at 21:02 -0700, Wei Hu wrote:
> Hi Alex,
> 
> With your change "Release devices with BusMaster disabled", I've found
> that my VFIO device driver is no longer receiving MSI interrupts.
> After reviewing the code I think it makes sense. But I had two
> questions below while debugging my issue.
> 
> 1.  If I had set the bus master bit in the command register by hand
> before opening the vfio device, the kernel would actually leave
> BusMaster enabled. This seems to contradict the call to
> pci_clear_master() from vfio_pci_enable(). What's going on here, is
> something else enabling BusMaster?

I don't see this behavior.  If I use a test program that simply opens
the vfio group, configures the IOMMU container and gets a device file
descriptor, BusMaster is clear at that point regardless of whether the
device has it enabled or disabled beforehand.  Are you perhaps attaching
bridges to vfio-pci and the bridge is getting enabled as part of the
pci_enable_device() call of the endpoint?  Newer versions of vfio-pci
won't allow binding bridges, it was a bug that it was ever allowed.  Our
config space manipulation only knows about normal header types.

> 2. What's the recommended way to enable BusMaster with your change
> now? Should my driver map the config space region and set the
> BusMaster bit? Or should I have a separate command to enable the bit
> before opening the device?

VFIO is designed to be a complete interface to the device, why would you
write a driver that makes use of VFIO that has pre-conditions of
manipulating the device state outside of VFIO?  There's no additional
mapping of config space required, you already have access to it through
the device file descriptor.  It should simply be a matter of doing a
read-modify-write at the proper offset to toggle the command register.
QEMU uses pread(2) and pwrite(2) for this, but any equivalent tool
should work as well.  Thanks,

Alex

Re: Correct way to enable BusMaster with VFIO?

[Wei Hu]

Hi Alex,

Thanks for your reply.

On Wed, Apr 8, 2015 at 8:17 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> Hi Wei,
>
> On Tue, 2015-04-07 at 21:02 -0700, Wei Hu wrote:
>> Hi Alex,
>>
>> With your change "Release devices with BusMaster disabled", I've found
>> that my VFIO device driver is no longer receiving MSI interrupts.
>> After reviewing the code I think it makes sense. But I had two
>> questions below while debugging my issue.
>>
>> 1.  If I had set the bus master bit in the command register by hand
>> before opening the vfio device, the kernel would actually leave
>> BusMaster enabled. This seems to contradict the call to
>> pci_clear_master() from vfio_pci_enable(). What's going on here, is
>> something else enabling BusMaster?
>
> I don't see this behavior.  If I use a test program that simply opens
> the vfio group, configures the IOMMU container and gets a device file
> descriptor, BusMaster is clear at that point regardless of whether the
> device has it enabled or disabled beforehand.  Are you perhaps attaching
> bridges to vfio-pci and the bridge is getting enabled as part of the
> pci_enable_device() call of the endpoint?  Newer versions of vfio-pci
> won't allow binding bridges, it was a bug that it was ever allowed.  Our
> config space manipulation only knows about normal header types.

Actually no, only two PCIe endpoints are assigned to vfio-pci. The
endpoints are two different functions on one PCIe device if that
matters.

>
>> 2. What's the recommended way to enable BusMaster with your change
>> now? Should my driver map the config space region and set the
>> BusMaster bit? Or should I have a separate command to enable the bit
>> before opening the device?
>
> VFIO is designed to be a complete interface to the device, why would you
> write a driver that makes use of VFIO that has pre-conditions of
> manipulating the device state outside of VFIO?  There's no additional
> mapping of config space required, you already have access to it through
> the device file descriptor.  It should simply be a matter of doing a
> read-modify-write at the proper offset to toggle the command register.
> QEMU uses pread(2) and pwrite(2) for this, but any equivalent tool
> should work as well.  Thanks,

Yes, I'll use pread/pwrite. Thank you!

Re: Correct way to enable BusMaster with VFIO?

[Alex Williamson]

On Wed, 2015-04-08 at 09:45 -0700, Wei Hu wrote:
> Hi Alex,
> 
> Thanks for your reply.
> 
> On Wed, Apr 8, 2015 at 8:17 AM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > Hi Wei,
> >
> > On Tue, 2015-04-07 at 21:02 -0700, Wei Hu wrote:
> >> Hi Alex,
> >>
> >> With your change "Release devices with BusMaster disabled", I've found
> >> that my VFIO device driver is no longer receiving MSI interrupts.
> >> After reviewing the code I think it makes sense. But I had two
> >> questions below while debugging my issue.
> >>
> >> 1.  If I had set the bus master bit in the command register by hand
> >> before opening the vfio device, the kernel would actually leave
> >> BusMaster enabled. This seems to contradict the call to
> >> pci_clear_master() from vfio_pci_enable(). What's going on here, is
> >> something else enabling BusMaster?
> >
> > I don't see this behavior.  If I use a test program that simply opens
> > the vfio group, configures the IOMMU container and gets a device file
> > descriptor, BusMaster is clear at that point regardless of whether the
> > device has it enabled or disabled beforehand.  Are you perhaps attaching
> > bridges to vfio-pci and the bridge is getting enabled as part of the
> > pci_enable_device() call of the endpoint?  Newer versions of vfio-pci
> > won't allow binding bridges, it was a bug that it was ever allowed.  Our
> > config space manipulation only knows about normal header types.
> 
> Actually no, only two PCIe endpoints are assigned to vfio-pci. The
> endpoints are two different functions on one PCIe device if that
> matters.

I'm not sure where that could be happening.  After calling
pci_clear_master() we call pci_enable_device(), which makes sure all the
device resources are programmed and the upstream bridges are enabled,
but does not touch BusMaster on the endpoint afaict.  We then attempt to
do a function-level reset on the device.  Even if the device were to not
adhere to the spec and have a reset value with BM enabled, we restore
config space after reset, which should disable it again and that's
pretty much how the device is provided to the user.  Could the device
not only exit reset with BM enabled, but also have an extremely long
reset latency that would not accept the command register restore?

That doesn't rule out that there could be SMM code on the platform that
re-enables BM.  If you clear BM and don't open the device through VFIO,
does it stay disabled?  That would be extremely evil SMM code, but I'm
out of ideas otherwise.  Thanks,

Alex

Re: Correct way to enable BusMaster with VFIO?

[Wei Hu]

On Wed, Apr 8, 2015 at 10:31 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Wed, 2015-04-08 at 09:45 -0700, Wei Hu wrote:
>> Hi Alex,
>>
>> Thanks for your reply.
>>
>> On Wed, Apr 8, 2015 at 8:17 AM, Alex Williamson
>> <alex.williamson@redhat.com> wrote:
>> > Hi Wei,
>> >
>> > On Tue, 2015-04-07 at 21:02 -0700, Wei Hu wrote:
>> >> Hi Alex,
>> >>
>> >> With your change "Release devices with BusMaster disabled", I've found
>> >> that my VFIO device driver is no longer receiving MSI interrupts.
>> >> After reviewing the code I think it makes sense. But I had two
>> >> questions below while debugging my issue.
>> >>
>> >> 1.  If I had set the bus master bit in the command register by hand
>> >> before opening the vfio device, the kernel would actually leave
>> >> BusMaster enabled. This seems to contradict the call to
>> >> pci_clear_master() from vfio_pci_enable(). What's going on here, is
>> >> something else enabling BusMaster?
>> >
>> > I don't see this behavior.  If I use a test program that simply opens
>> > the vfio group, configures the IOMMU container and gets a device file
>> > descriptor, BusMaster is clear at that point regardless of whether the
>> > device has it enabled or disabled beforehand.  Are you perhaps attaching
>> > bridges to vfio-pci and the bridge is getting enabled as part of the
>> > pci_enable_device() call of the endpoint?  Newer versions of vfio-pci
>> > won't allow binding bridges, it was a bug that it was ever allowed.  Our
>> > config space manipulation only knows about normal header types.
>>
>> Actually no, only two PCIe endpoints are assigned to vfio-pci. The
>> endpoints are two different functions on one PCIe device if that
>> matters.
>
> I'm not sure where that could be happening.  After calling
> pci_clear_master() we call pci_enable_device(), which makes sure all the
> device resources are programmed and the upstream bridges are enabled,
> but does not touch BusMaster on the endpoint afaict.  We then attempt to
> do a function-level reset on the device.  Even if the device were to not
> adhere to the spec and have a reset value with BM enabled, we restore
> config space after reset, which should disable it again and that's
> pretty much how the device is provided to the user.  Could the device
> not only exit reset with BM enabled, but also have an extremely long
> reset latency that would not accept the command register restore?
>
> That doesn't rule out that there could be SMM code on the platform that
> re-enables BM.  If you clear BM and don't open the device through VFIO,
> does it stay disabled?  That would be extremely evil SMM code, but I'm
> out of ideas otherwise.  Thanks,
>
> Alex
>

If I clear BM then it stays cleared, whether or not I open the device
through VFIO. If I set BM then it stays set, only after VFIO release
does BM gets cleared.