Re: [PATCH v2 01/11] hw: encode accessing CPU index in MemTxAttrs

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 26 Sept 2022 at 15:13, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> We currently have hacks across the hw/ to reference current_cpu to
>> work out what the current accessing CPU is. This breaks in some cases
>> including using gdbstub to access HW state. As we have MemTxAttrs to
>> describe details about the access lets extend it to mention if this is
>> a CPU access and which one it is.
>>
>> There are a number of places we need to fix up including:
>>
>>   CPU helpers directly calling address_space_*() fns
>>   models in hw/ fishing the data out of current_cpu
>>   hypervisors offloading device emulation to QEMU
>>
>> I'll start addressing some of these in following patches.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v2
>>   - use separate field cpu_index
>>   - bool for requester_is_cpu
>> v3
>>   - switch to enum MemTxRequesterType
>>   - move helper #define to patch
>>   - revert to overloading requester_id
>>   - mention hypervisors in commit message
>>   - drop cputlb tweaks, they will move to target specific code
>
> I still don't see anything in this patchset that updates
> the code which currently assumes requester_id to be a PCI
> index to check that it hasn't been handed a MemTxAttrs
> that uses requester_id as a CPU number.

OK I'll update so all the existing cases setting requester_id also set
the type to MEMTXATTRS_MSI.

I also noticed the GIC ITS code checks requester ID. Should we assert
(or hw_error?) if it's not the case?

> I also still need to go and look up how hardware does this,
> so please don't queue this patchset yet. In particular, we
> should think about whether we want this to be:
>  * a CPU number, but only set opt-in by some target archs

Given a whole bunch of arches currently use MEMTXATTRS_UNSPECIFIED I
think for now it's worth confining to just ARM where we know we have
devices that care about the cpu_index and have tagged the various paths
with the correct data.

>  * a CPU number, valid for all target archs
>  * a unique-within-the-machine identifier of the transaction
>    master (i.e. which can be set by DMA controllers, etc,
>    not just CPUs)

That would require something to keep a map of requester_id's to
source/index right?

> I would also like some input from Edgar since I know Xilinx
> have some more extensive out-of-tree uses of requester_id.
> We aren't obligated to not break out-of-tree code, but that
> seems like a bunch of experience and knowledge about how
> real hardware works that would be useful for informing
> how we design this.

His comment against the last iteration was:

"CPU's can also have a Master-IDs (Requester IDs) which are unrelated to
the Clusters CPU index. This is used for example in the Xilinx ZynqMP
and Xilinx Versal and the XMPU (Memory Protection Units).

Anyway, I think this approach is an improvement from the current state
but would rather see a new separate field from requester_id. Overloading
requester_id will break some of our use-cases (in the Xilinx tree)...

IIRC a real GIC differentiates between the connected CPU's through
different ports, not by looking at master-ids but I'm not 100% sure..."

at the same time Richard's not keen about adding extra fields
(especially as some arches have INT32_MAX bounds for cpu_index). However
one approach would be to expand the requester_id field and you could
then expand MemTxRequesterType to and have a multiplexed type although I
admit it's hard to imagine HW that cares about both the CPU and bus id
at the same time.

>
> thanks
> -- PMM


-- 
Alex Bennée