Re: [RFC PATCH 3/5] hw/pci-host/bonito: Remap PCI "lo" regions when PCIMAP reg is modified

[BALATON Zoltan via <qemu-devel@nongnu.org>]

[-- Attachment #1: Type: text/plain, Size: 4365 bytes --]

On Sat, 2 Jan 2021, Philippe Mathieu-Daudé wrote:
> On 1/2/21 12:19 AM, Peter Maydell wrote:
>> On Fri, 1 Jan 2021 at 23:12, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> Per the datasheet (Chapter 5.7.1. "PCI address regions"),
>>> the PCIMAP register:
>>>
>>>   Map the 64Mbyte regions marked "PCI_Lo" in the CPU's memory map,
>>>   each of which can be assigned to any 64 Mbyte-aligned region of
>>>   PCI memory. The address appearing on the PCI bus consists of the
>>>   low 26 bits of the CPU physical address, with the high 6 bits
>>>   coming from the appropriate base6 field. Each of the three regions
>>>   is an independent window onto PCI memory, and can be positioned on
>>>   any 64Mbyte boundary in PCI space.
>>>
>>> Remap the 3 regions on reset and when PCIMAP is updated.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/pci-host/bonito.c | 49 ++++++++++++++++++++++++++++++++------------
>>>  1 file changed, 36 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
>>> index a99eced0657..c58eeaf504c 100644
>>> --- a/hw/pci-host/bonito.c
>>> +++ b/hw/pci-host/bonito.c
>>> @@ -137,6 +137,10 @@ FIELD(BONGENCFG, PCIQUEUE,      12, 1)
>>>
>>>  /* 4. PCI address map control */
>>>  #define BONITO_PCIMAP           (0x10 >> 2)      /* 0x110 */
>>> +FIELD(PCIMAP, LO0,               0, 6)
>>> +FIELD(PCIMAP, LO1,               6, 6)
>>> +FIELD(PCIMAP, LO2,              12, 6)
>>> +FIELD(PCIMAP, 2G,               18, 1)
>>>  #define BONITO_PCIMEMBASECFG    (0x14 >> 2)      /* 0x114 */
>>>  #define BONITO_PCIMAP_CFG       (0x18 >> 2)      /* 0x118 */
>>>
>>> @@ -237,6 +241,7 @@ struct BonitoState {
>>>      qemu_irq *pic;
>>>      PCIBonitoState *pci_dev;
>>>      MemoryRegion pci_mem;
>>> +    MemoryRegion pcimem_lo_alias[3];
>>>  };
>>>
>>>  #define TYPE_BONITO_PCI_HOST_BRIDGE "Bonito-pcihost"
>>> @@ -245,6 +250,31 @@ OBJECT_DECLARE_SIMPLE_TYPE(BonitoState, BONITO_PCI_HOST_BRIDGE)
>>>  #define TYPE_PCI_BONITO "Bonito"
>>>  OBJECT_DECLARE_SIMPLE_TYPE(PCIBonitoState, PCI_BONITO)
>>>
>>> +static void bonito_remap(PCIBonitoState *s)
>>> +{
>>> +    static const char *const region_name[3] = {
>>> +        "pci.lomem0", "pci.lomem1", "pci.lomem2"
>>> +    };
>>> +    BonitoState *bs = BONITO_PCI_HOST_BRIDGE(s->pcihost);
>>> +
>>> +    for (size_t i = 0; i < 3; i++) {
>>> +        uint32_t offset = extract32(s->regs[BONITO_PCIMAP], 6 * i, 6) << 26;
>>> +
>>> +        if (memory_region_is_mapped(&bs->pcimem_lo_alias[i])) {
>>> +            memory_region_del_subregion(get_system_memory(),
>>> +                                        &bs->pcimem_lo_alias[i]);
>>> +            object_unparent(OBJECT(&bs->pcimem_lo_alias[i]));
>>> +        }
>>> +
>>> +        memory_region_init_alias(&bs->pcimem_lo_alias[i], OBJECT(s),
>>> +                                 region_name[i], &bs->pci_mem,
>>> +                                 offset, 64 * MiB);
>>> +        memory_region_add_subregion(get_system_memory(),
>>> +                                    BONITO_PCILO_BASE + i * 64 * MiB,
>>> +                                    &bs->pcimem_lo_alias[i]);
>>> +    }
>>
>> Rather than delete-and-reinit-and-add, it's probably better to
>> just create the subregions once at device startup, and then use
>> memory_region_set_enabled() and memory_region_set_address()
>> to manipulate whether the subregion is visible and what address
>> in the system memory it is mapped at.
>
> Great! Thanks Peter :) TIL these functions.
> From what I understand from memory_region_readd_subregion (called
> from memory_region_set_address) using memory_region_set_enabled()
> directly is enough.

I have similar code in the series I've just posted where I'm mapping 
regions of serial devices. I did consider using set_enabled and 
set_address but ended up with removing and adding regions because I'm not 
sure what happens if guest tries to move one region over another like 
having one region at a default location while guest tries to map the other 
one there (the pegasos2 maps serial at 0x2f8 which is normally COM2 on a 
PC). This should not happen in theory but when removing disabled regions 
it cannot happen so that looks safer therefore I chose to do that. Not 
sure if this could be a problem here just shared my thughts about this.

Regards,
BALATON Zoltan