Re: [Xen-devel] [PATCH v4 1/5] vpci: fix updating the command register

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.11.18 at 12:09, <roger.pau@xxxxxxxxxx> wrote:
> On Mon, Nov 19, 2018 at 01:26:11AM -0700, Jan Beulich wrote:
>> >>> On 16.11.18 at 15:32, <roger.pau@xxxxxxxxxx> wrote:
>> > On Fri, Nov 16, 2018 at 05:00:29AM -0700, Jan Beulich wrote:
>> >> >>> On 14.11.18 at 12:57, <roger.pau@xxxxxxxxxx> wrote:
>> >> > @@ -413,7 +412,7 @@ static void rom_write(const struct pci_dev *pdev, 
>> >> > unsigned int reg,
>> >> >          header->rom_enabled = new_enabled;
>> >> >          pci_conf_write32(pdev->seg, pdev->bus, slot, func, reg, val);
>> >> >      }
>> >> > -    else if ( modify_bars(pdev, new_enabled, true) )
>> >> > +    else if ( modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, 
>> >> > true) )
>> >> 
>> >> Do you really mean to clear all other defined bits of the command
>> >> register here?
>> > 
>> > This is a ROM BAR write, not a command register write. rom_write
>> > passes PCI_COMMAND_MEMORY merely to signal this is a mapping
>> > operation, but the value would never be written to the command
>> > register, there's an ASSERT(!rom_only) just before the deferred write
>> > of the command register in modify_decoding.
>> 
>> Oh, I see. This is getting more subtle than it already was, so perhaps
>> worth attaching a brief comment here, the more that if anything was
>> wrong with the logic bad behavior would result in release builds?
> 
> It would, I could change the ASSERT to a BUG in modify_decoding if
> that seems more foolproof.

I intentionally did not suggest such a conversion: I'd like us to
stop bringing down the entire host when a problem related to
just one guest is encountered. That said - as long as vPCI is
for the hardware domain only, the transformation (annotated
to indicate it needs adjustment when widening exposure to
DomU-s) would perhaps be acceptable.

Jan



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