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 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.

Jan