Re: [PATCH for-4.17 v2 5/5] vpci: refuse BAR writes only if the BAR is mapped

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.10.2022 18:01, Roger Pau Monné wrote:
> On Wed, Oct 26, 2022 at 04:06:40PM +0200, Jan Beulich wrote:
>> On 26.10.2022 15:58, Roger Pau Monné wrote:
>>> On Wed, Oct 26, 2022 at 02:47:43PM +0200, Jan Beulich wrote:
>>>> On 25.10.2022 16:44, Roger Pau Monne wrote:
>>>>> @@ -388,12 +391,12 @@ static void cf_check bar_write(
>>>>>      else
>>>>>          val &= PCI_BASE_ADDRESS_MEM_MASK;
>>>>>  
>>>>> -    if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
>>>>> +    if ( bar->enabled )
>>>>
>>>> In 3 of the 4 cases you use header->bars_mapped as replacement. Since it's
>>>> not clear to me why you don't here, could you explain this to me? (I'm
>>>> therefore undecided whether this is merely a cosmetic [consistency] issue.)
>>>
>>> No, it's intended to use bar->enabled here rather than
>>> header->bars_mapped.
>>>
>>> It's possible to have header->bars_mapped == true, but bar->enabled ==
>>> false if memory decoding is enabled, but this BAR specifically has
>>> failed to be mapped in the guest p2m, which means dom0 is safe to move
>>> it for what Xen cares (ie: it won't mess with p2m mappings because
>>> there are none for the BAR).
>>>
>>> We could be more strict and use header->bars_mapped, but I don't think
>>> there's a need for it.
>>>
>>> What about adding a comment with:
>>>
>>> /*
>>>  * Xen only cares whether the BAR is mapped into the p2m, so allow BAR
>>>  * writes as long as the BAR is not mapped into the p2m.
>>>  */
>>>
>>> Otherwise I can switch to using header->bars_mapped if you think
>>> that's clearer.
>>
>> It's not so much a matter of being clearer, but a matter of consistency:
>> Why does the same consideration not apply in rom_write()? In fact both
>> uses there are (already before the change) combined with further
>> conditions (checking header->rom_enabled and new_enabled). If the
>> inconsistency is on purpose (and perhaps necessary), I'd like to first
>> understand why that is before deciding what to do about it. A comment
>> like you suggest it _may_ be the route to go.
> 
> ROM register is more complex to handle, because the same register
> that's used to store the address also contains the bit that can
> trigger whether it's mapped into the guest p2m or not
> (PCI_ROM_ADDRESS_ENABLE).  So ROM BAR writes with the ROM BAR mapped
> and the PCI_ROM_ADDRESS_ENABLE bit also set in the to be written value
> will be rejected, because we only allow to first disable the ROM and
> then change the address (which is likely to not play well with OSes,
> but so far I haven't been able to test ROM BAR register usage on PVH).
> 
> I do think for consistency it would be better to use rom->enabled in
> the first conditional of rom_write() check, so it would be:
> 
>     if ( rom->enabled && new_enabled )
>     {
>         gprintk(XENLOG_WARNING,
>                 "%pp: ignored ROM BAR write while mapped\n",
>                 &pdev->sbdf);
>         return;
>     }
> 
> So that we also allow changing the address of ROM BARs even with
> memory decoding and PCI_ROM_ADDRESS_ENABLE as long as the ROM BAR is
> not mapped in the p2m.
> 
> Would you be fine with the comment in the previous email added and
> rom_write() adjusted as suggested above?

Yes, that would look better to me. The comment then probably also wants
duplicating (or pointing at from) here.

Jan