Re: [Xen-devel] [PATCH v3 4/7] vpci: fix updating the command register

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Nov 05, 2018 at 09:46:22AM -0700, Jan Beulich wrote:
> >>> On 30.10.18 at 16:41, <roger.pau@xxxxxxxxxx> wrote:
> > When switching the memory decoding bit in the command register the
> > rest of the changes where dropped, leading to only the memory decoding
> > bit being updated.
> > 
> > Fix this by unconditionally writing the guest-requested command except
> > for the memory decoding bit, which will be updated once the p2m
> > changes are performed.
> 
> Are you convinced there are no devices (or drivers) with errata
> requiring the write to happen in a single step?

That I certainly don't know. On Xen we already toggle the memory
decoding bit if required when sizing the BARs during initialization,
which will likely also trigger such a bug if it exist.

> Remember that
> the register value immediately becomes visible to other (v)CPUs.

But the vCPU that performed the write would be blocked and only
released once all the bits have been updated. While other vCPUs could
indeed see a partly updated command value without the memory decode
bit set I'm not sure this is harmful, well behaved OSes should wait
for the write to complete in any case.

> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -333,8 +333,10 @@ static void cmd_write(const struct pci_dev *pdev, 
> > unsigned int reg,
> >           * hoping the guest will realize and try again.
> >           */
> >          modify_bars(pdev, cmd & PCI_COMMAND_MEMORY, false);
> > -    else
> > -        pci_conf_write16(pdev->seg, pdev->bus, slot, func, reg, cmd);
> > +
> > +    /* Write the new command without updating the memory decoding bit. */
> > +    cmd = (cmd & ~PCI_COMMAND_MEMORY) | (current_cmd & PCI_COMMAND_MEMORY);
> > +    pci_conf_write16(pdev->seg, pdev->bus, slot, func, reg, cmd);
> 
> Furthermore, if the mapping didn't get deferred, aren't you
> discarding the new decode bit value here, as written by
> modify_bars() -> apply_map() -> modify_decoding()?

The map should always be deferred when called from cmd_write because
this handler will be accessed exclusively by the guest, in which case
system_state >= SYS_STATE_active.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel