Re: [PATCH v4 3/8] hw/intc: GICv3 ITS command queue framework
Hi Eric,
Please find my responses to your latest comments (taken care of)
inline:-
On Mon, 2021-06-21 at 12:03 +0200, Eric Auger wrote:
>
> On 6/16/21 11:02 PM, shashi.mall...@linaro.org wrote:
> > Hi Eric,
> >
> > Please find my responses inline (below):-
> >
> > On Sun, 2021-06-13 at 16:13 +0200, Eric Auger wrote:
> > > Hi Sashi,
> > >
> > > On 6/2/21 8:00 PM, Shashi Mallela wrote:
> > > > Added functionality to trigger ITS command queue processing on
> > > > write to CWRITE register and process each command queue entry
> > > > to
> > > > identify the command type and handle commands like
> > > > MAPD,MAPC,SYNC.
> > > >
> > > > Signed-off-by: Shashi Mallela
> > > > ---
> > > > hw/intc/arm_gicv3_its.c | 295
> > > > +++
> > > > hw/intc/gicv3_internal.h | 37 +
> > > > 2 files changed, 332 insertions(+)
> > > >
> > > > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> > > > index af60f19c98..6551c577b3 100644
> > > > --- a/hw/intc/arm_gicv3_its.c
> > > > +++ b/hw/intc/arm_gicv3_its.c
> > > > @@ -49,6 +49,295 @@ static uint64_t baser_base_addr(uint64_t
> > > > value,
> > > > uint32_t page_sz)
> > > > return result;
> > > > }
> > > >
> > > > +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid,
> > > > bool valid,
> > > > + uint64_t rdbase)
> > > > +{
> > > > +AddressSpace *as = >gicv3->dma_as;
> > > > +uint64_t value;
> > > > +uint64_t l2t_addr;
> > > > +bool valid_l2t;
> > > > +uint32_t l2t_id;
> > > > +uint32_t max_l2_entries;
> > > > +uint64_t cte = 0;
> > > > +MemTxResult res = MEMTX_OK;
> > > > +
> > > > +if (!s->ct.valid) {
> > > Isn't it a guest log error case. Also you return MEMTX_OK in that
> > > case.
> > > Is that what you want?
> > Yes,because the current implementation treats all command specific
> > errors as "ignored" and moves onto next command in the queue.MEMTX
> > return values are significant for dma read/write status and in case
> > of
> > error we stall the command processing
> OK
> > > > +return res;
> > > > +}
> > > > +
> > > > +if (valid) {
> > > > +/* add mapping entry to collection table */
> > > > +cte = (valid & VALID_MASK) |
> > > > + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL);
> > > Do you really need to sanitize rdbase again?
> > Not required,have rectified it.
> > > > +}
> > > > +
> > > > +/*
> > > > + * The specification defines the format of level 1 entries
> > > > of
> > > > a
> > > > + * 2-level table, but the format of level 2 entries and
> > > > the
> > > > format
> > > > + * of flat-mapped tables is IMPDEF.
> > > > + */
> > > > +if (s->ct.indirect) {
> > > > +l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE);
> > > > +
> > > > +value = address_space_ldq_le(as,
> > > > + s->ct.base_addr +
> > > > + (l2t_id *
> > > > L1TABLE_ENTRY_SIZE),
> > > > + MEMTXATTRS_UNSPECIFIED,
> > > > );
> > > > +
> > > > +if (res != MEMTX_OK) {
> > > > +return res;
> > > > +}
> > > > +
> > > > +valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
> > > > +
> > > > +if (valid_l2t) {
> > > > +max_l2_entries = s->ct.page_sz / s->ct.entry_sz;
> > > > +
> > > > +l2t_addr = value & ((1ULL << 51) - 1);
> > > > +
> > > > +address_space_stq_le(as, l2t_addr +
> > > > + ((icid % max_l2_entries) *
> > > > GITS_CTE_SIZE),
> > > > + cte, MEMTXATTRS_UNSPECIFIED,
> > > > );
> > > > +}
> > > > +} else {
> > > > +/* Flat level table */
> > > > +address_space_stq_le(as, s->ct.base_addr + (icid *
> > > > GITS_CTE_SIZE),
> > > > + cte, MEMTXATTRS_UNSPECIFIED,
> > > > );
> > > > +}
> > > > +return res;
> > > > +}
> > > > +
> > > > +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t
> > > > offset)
> > > > +{
> > > > +AddressSpace *as = >gicv3->dma_as;
> > > > +uint16_t icid;
> > > > +uint64_t rdbase;
> > > > +bool valid;
> > > > +MemTxResult res = MEMTX_OK;
> > > > +uint64_t value;
> > > > +
> > > > +offset += NUM_BYTES_IN_DW;
> > > > +offset += NUM_BYTES_IN_DW;
> > > May be relevant to add some trace points for debuggability.
> > Probably the trace functionality for ITS can be taken up as a
> > seperate
> > task/feature TODO.
> Yes of course. It may just be useful for you as well to debug ;-)
> > > > +
> > > > +value = address_space_ldq_le(as, s->cq.base_addr + offset,
> > > > + MEMTXATTRS_UNSPECIFIED,
> > > > );
> > > > +
> > > > +if (res != MEMTX_OK) {
> > > > +return res;
> > > > +}
> > > > +
> > > > +icid = value & ICID_MASK;
> > > > +
> > > > +rdbase = (value >>
Re: [PATCH v4 3/8] hw/intc: GICv3 ITS command queue framework
Hi Eric,
Had missed this comment earlier.Please find my response (inline)below:-
On Sun, 2021-06-13 at 16:39 +0200, Eric Auger wrote:
> Hi,
>
> On 6/2/21 8:00 PM, Shashi Mallela wrote:
> > Added functionality to trigger ITS command queue processing on
> > write to CWRITE register and process each command queue entry to
> > identify the command type and handle commands like MAPD,MAPC,SYNC.
> >
> > Signed-off-by: Shashi Mallela
> > ---
> > hw/intc/arm_gicv3_its.c | 295
> > +++
> > hw/intc/gicv3_internal.h | 37 +
> > 2 files changed, 332 insertions(+)
> >
> > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> > index af60f19c98..6551c577b3 100644
> > --- a/hw/intc/arm_gicv3_its.c
> > +++ b/hw/intc/arm_gicv3_its.c
> > @@ -49,6 +49,295 @@ static uint64_t baser_base_addr(uint64_t value,
> > uint32_t page_sz)
> > return result;
> > }
> >
> > +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid,
> > bool valid,
> > + uint64_t rdbase)
> > +{
> > +AddressSpace *as = >gicv3->dma_as;
> > +uint64_t value;
> > +uint64_t l2t_addr;
> > +bool valid_l2t;
> > +uint32_t l2t_id;
> > +uint32_t max_l2_entries;
> > +uint64_t cte = 0;
> > +MemTxResult res = MEMTX_OK;
> > +
> > +if (!s->ct.valid) {
> > +return res;
> > +}
> > +
> > +if (valid) {
> > +/* add mapping entry to collection table */
> > +cte = (valid & VALID_MASK) |
> > + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL);
> > +}
> > +
> > +/*
> > + * The specification defines the format of level 1 entries of
> > a
> > + * 2-level table, but the format of level 2 entries and the
> > format
> > + * of flat-mapped tables is IMPDEF.
> > + */
> > +if (s->ct.indirect) {
> > +l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE);
> > +
> > +value = address_space_ldq_le(as,
> > + s->ct.base_addr +
> > + (l2t_id *
> > L1TABLE_ENTRY_SIZE),
> > + MEMTXATTRS_UNSPECIFIED,
> > );
> > +
> > +if (res != MEMTX_OK) {
> > +return res;
> > +}
> > +
> > +valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
> > +
> > +if (valid_l2t) {
> > +max_l2_entries = s->ct.page_sz / s->ct.entry_sz;
> > +
> > +l2t_addr = value & ((1ULL << 51) - 1);
> > +
> > +address_space_stq_le(as, l2t_addr +
> > + ((icid % max_l2_entries) *
> > GITS_CTE_SIZE),
> > + cte, MEMTXATTRS_UNSPECIFIED,
> > );
> > +}
> > +} else {
> > +/* Flat level table */
> > +address_space_stq_le(as, s->ct.base_addr + (icid *
> > GITS_CTE_SIZE),
> > + cte, MEMTXATTRS_UNSPECIFIED, );
> > +}
> > +return res;
> > +}
>
> Looking at your DevTableDesc and CollTableDesc types again, they are
> basically the same except max_devids/max_collids. You may use a
> single
> one and it may be possible to define helpers to access an entry in
> the
> DT or CT.
will replace DevTableDesc/CollTableDesc types with a common TableDesc
type and introduce a new union member to hold one of
max_devids/max_collids to be referenced by all relevant functions.
>
> update_cte/update_dte looks quite similar if you compute the cte and
> dte
> externally and pass a pointer to the associated TableDesc?
update_cte/update_cte will (continue) to reference their respective
tables via existing Gicv3ItsState type but through the newly defined
common TableDesc type.
Hope this helps.
>
> Thanks
>
> Eric
>
>
> > +
> > +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset)
> > +{
> > +AddressSpace *as = >gicv3->dma_as;
> > +uint16_t icid;
> > +uint64_t rdbase;
> > +bool valid;
> > +MemTxResult res = MEMTX_OK;
> > +uint64_t value;
> > +
> > +offset += NUM_BYTES_IN_DW;
> > +offset += NUM_BYTES_IN_DW;
> > +
> > +value = address_space_ldq_le(as, s->cq.base_addr + offset,
> > + MEMTXATTRS_UNSPECIFIED, );
> > +
> > +if (res != MEMTX_OK) {
> > +return res;
> > +}
> > +
> > +icid = value & ICID_MASK;
> > +
> > +rdbase = (value >> R_MAPC_RDBASE_SHIFT) & RDBASE_PROCNUM_MASK;
> > +
> > +valid = (value >> VALID_SHIFT) & VALID_MASK;
> > +
> > +if ((icid > s->ct.max_collids) || (rdbase > s->gicv3-
> > >num_cpu)) {
> > +qemu_log_mask(LOG_GUEST_ERROR,
> > + "ITS MAPC: invalid collection table
> > attributes "
> > + "icid %d rdbase %lu\n", icid, rdbase);
> > +/*
> > + * in this implementation,in case of error
> > + * we ignore this command and move onto the next
> > + * command in the queue
> > + */
> > +} else {
> > +res = update_cte(s, icid, valid,
Re: [PATCH v4 3/8] hw/intc: GICv3 ITS command queue framework
On 6/16/21 11:02 PM, shashi.mall...@linaro.org wrote:
> Hi Eric,
>
> Please find my responses inline (below):-
>
> On Sun, 2021-06-13 at 16:13 +0200, Eric Auger wrote:
>> Hi Sashi,
>>
>> On 6/2/21 8:00 PM, Shashi Mallela wrote:
>>> Added functionality to trigger ITS command queue processing on
>>> write to CWRITE register and process each command queue entry to
>>> identify the command type and handle commands like MAPD,MAPC,SYNC.
>>>
>>> Signed-off-by: Shashi Mallela
>>> ---
>>> hw/intc/arm_gicv3_its.c | 295
>>> +++
>>> hw/intc/gicv3_internal.h | 37 +
>>> 2 files changed, 332 insertions(+)
>>>
>>> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
>>> index af60f19c98..6551c577b3 100644
>>> --- a/hw/intc/arm_gicv3_its.c
>>> +++ b/hw/intc/arm_gicv3_its.c
>>> @@ -49,6 +49,295 @@ static uint64_t baser_base_addr(uint64_t value,
>>> uint32_t page_sz)
>>> return result;
>>> }
>>>
>>> +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid,
>>> bool valid,
>>> + uint64_t rdbase)
>>> +{
>>> +AddressSpace *as = >gicv3->dma_as;
>>> +uint64_t value;
>>> +uint64_t l2t_addr;
>>> +bool valid_l2t;
>>> +uint32_t l2t_id;
>>> +uint32_t max_l2_entries;
>>> +uint64_t cte = 0;
>>> +MemTxResult res = MEMTX_OK;
>>> +
>>> +if (!s->ct.valid) {
>> Isn't it a guest log error case. Also you return MEMTX_OK in that
>> case.
>> Is that what you want?
> Yes,because the current implementation treats all command specific
> errors as "ignored" and moves onto next command in the queue.MEMTX
> return values are significant for dma read/write status and in case of
> error we stall the command processing
OK
>>> +return res;
>>> +}
>>> +
>>> +if (valid) {
>>> +/* add mapping entry to collection table */
>>> +cte = (valid & VALID_MASK) |
>>> + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL);
>> Do you really need to sanitize rdbase again?
> Not required,have rectified it.
>>> +}
>>> +
>>> +/*
>>> + * The specification defines the format of level 1 entries of
>>> a
>>> + * 2-level table, but the format of level 2 entries and the
>>> format
>>> + * of flat-mapped tables is IMPDEF.
>>> + */
>>> +if (s->ct.indirect) {
>>> +l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE);
>>> +
>>> +value = address_space_ldq_le(as,
>>> + s->ct.base_addr +
>>> + (l2t_id *
>>> L1TABLE_ENTRY_SIZE),
>>> + MEMTXATTRS_UNSPECIFIED,
>>> );
>>> +
>>> +if (res != MEMTX_OK) {
>>> +return res;
>>> +}
>>> +
>>> +valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
>>> +
>>> +if (valid_l2t) {
>>> +max_l2_entries = s->ct.page_sz / s->ct.entry_sz;
>>> +
>>> +l2t_addr = value & ((1ULL << 51) - 1);
>>> +
>>> +address_space_stq_le(as, l2t_addr +
>>> + ((icid % max_l2_entries) *
>>> GITS_CTE_SIZE),
>>> + cte, MEMTXATTRS_UNSPECIFIED,
>>> );
>>> +}
>>> +} else {
>>> +/* Flat level table */
>>> +address_space_stq_le(as, s->ct.base_addr + (icid *
>>> GITS_CTE_SIZE),
>>> + cte, MEMTXATTRS_UNSPECIFIED, );
>>> +}
>>> +return res;
>>> +}
>>> +
>>> +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset)
>>> +{
>>> +AddressSpace *as = >gicv3->dma_as;
>>> +uint16_t icid;
>>> +uint64_t rdbase;
>>> +bool valid;
>>> +MemTxResult res = MEMTX_OK;
>>> +uint64_t value;
>>> +
>>> +offset += NUM_BYTES_IN_DW;
>>> +offset += NUM_BYTES_IN_DW;
>> May be relevant to add some trace points for debuggability.
> Probably the trace functionality for ITS can be taken up as a seperate
> task/feature TODO.
Yes of course. It may just be useful for you as well to debug ;-)
>>> +
>>> +value = address_space_ldq_le(as, s->cq.base_addr + offset,
>>> + MEMTXATTRS_UNSPECIFIED, );
>>> +
>>> +if (res != MEMTX_OK) {
>>> +return res;
>>> +}
>>> +
>>> +icid = value & ICID_MASK;
>>> +
>>> +rdbase = (value >> R_MAPC_RDBASE_SHIFT) & RDBASE_PROCNUM_MASK;
>> usually the mask is applied before the shift.
> Here we are extracting only 16 bit rdbase(processor number) value by
> masking with RDBASE_PROCNUM_MASK only after we have right shifted the
> rdbase offset from the 64 bit DW value.
> As an alternative,I could have used rdbase = (value &
> R_MAPC_RDBASE_MASK) to first extract the 32 bits rdbase value from DW
> and then later mask again with RDBASE_PROCNUM_MASK to narrow it down to
> 16 bit rdbase(processor number).
My comment rather was about the fact that generally the mask applied to
the shifted location and then you shift the masked field. I notived
Peter also made this comment in 4/8 (FIELD
Re: [PATCH v4 3/8] hw/intc: GICv3 ITS command queue framework
Hi Eric,
Please find my responses inline (below):-
On Sun, 2021-06-13 at 16:13 +0200, Eric Auger wrote:
> Hi Sashi,
>
> On 6/2/21 8:00 PM, Shashi Mallela wrote:
> > Added functionality to trigger ITS command queue processing on
> > write to CWRITE register and process each command queue entry to
> > identify the command type and handle commands like MAPD,MAPC,SYNC.
> >
> > Signed-off-by: Shashi Mallela
> > ---
> > hw/intc/arm_gicv3_its.c | 295
> > +++
> > hw/intc/gicv3_internal.h | 37 +
> > 2 files changed, 332 insertions(+)
> >
> > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> > index af60f19c98..6551c577b3 100644
> > --- a/hw/intc/arm_gicv3_its.c
> > +++ b/hw/intc/arm_gicv3_its.c
> > @@ -49,6 +49,295 @@ static uint64_t baser_base_addr(uint64_t value,
> > uint32_t page_sz)
> > return result;
> > }
> >
> > +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid,
> > bool valid,
> > + uint64_t rdbase)
> > +{
> > +AddressSpace *as = >gicv3->dma_as;
> > +uint64_t value;
> > +uint64_t l2t_addr;
> > +bool valid_l2t;
> > +uint32_t l2t_id;
> > +uint32_t max_l2_entries;
> > +uint64_t cte = 0;
> > +MemTxResult res = MEMTX_OK;
> > +
> > +if (!s->ct.valid) {
> Isn't it a guest log error case. Also you return MEMTX_OK in that
> case.
> Is that what you want?
Yes,because the current implementation treats all command specific
errors as "ignored" and moves onto next command in the queue.MEMTX
return values are significant for dma read/write status and in case of
error we stall the command processing
> > +return res;
> > +}
> > +
> > +if (valid) {
> > +/* add mapping entry to collection table */
> > +cte = (valid & VALID_MASK) |
> > + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL);
> Do you really need to sanitize rdbase again?
Not required,have rectified it.
> > +}
> > +
> > +/*
> > + * The specification defines the format of level 1 entries of
> > a
> > + * 2-level table, but the format of level 2 entries and the
> > format
> > + * of flat-mapped tables is IMPDEF.
> > + */
> > +if (s->ct.indirect) {
> > +l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE);
> > +
> > +value = address_space_ldq_le(as,
> > + s->ct.base_addr +
> > + (l2t_id *
> > L1TABLE_ENTRY_SIZE),
> > + MEMTXATTRS_UNSPECIFIED,
> > );
> > +
> > +if (res != MEMTX_OK) {
> > +return res;
> > +}
> > +
> > +valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
> > +
> > +if (valid_l2t) {
> > +max_l2_entries = s->ct.page_sz / s->ct.entry_sz;
> > +
> > +l2t_addr = value & ((1ULL << 51) - 1);
> > +
> > +address_space_stq_le(as, l2t_addr +
> > + ((icid % max_l2_entries) *
> > GITS_CTE_SIZE),
> > + cte, MEMTXATTRS_UNSPECIFIED,
> > );
> > +}
> > +} else {
> > +/* Flat level table */
> > +address_space_stq_le(as, s->ct.base_addr + (icid *
> > GITS_CTE_SIZE),
> > + cte, MEMTXATTRS_UNSPECIFIED, );
> > +}
> > +return res;
> > +}
> > +
> > +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset)
> > +{
> > +AddressSpace *as = >gicv3->dma_as;
> > +uint16_t icid;
> > +uint64_t rdbase;
> > +bool valid;
> > +MemTxResult res = MEMTX_OK;
> > +uint64_t value;
> > +
> > +offset += NUM_BYTES_IN_DW;
> > +offset += NUM_BYTES_IN_DW;
> May be relevant to add some trace points for debuggability.
Probably the trace functionality for ITS can be taken up as a seperate
task/feature TODO.
> > +
> > +value = address_space_ldq_le(as, s->cq.base_addr + offset,
> > + MEMTXATTRS_UNSPECIFIED, );
> > +
> > +if (res != MEMTX_OK) {
> > +return res;
> > +}
> > +
> > +icid = value & ICID_MASK;
> > +
> > +rdbase = (value >> R_MAPC_RDBASE_SHIFT) & RDBASE_PROCNUM_MASK;
> usually the mask is applied before the shift.
Here we are extracting only 16 bit rdbase(processor number) value by
masking with RDBASE_PROCNUM_MASK only after we have right shifted the
rdbase offset from the 64 bit DW value.
As an alternative,I could have used rdbase = (value &
R_MAPC_RDBASE_MASK) to first extract the 32 bits rdbase value from DW
and then later mask again with RDBASE_PROCNUM_MASK to narrow it down to
16 bit rdbase(processor number).
> > +
> > +valid = (value >> VALID_SHIFT) & VALID_MASK;
> use FIELD, see below
> > +
> > +if ((icid > s->ct.max_collids) || (rdbase > s->gicv3-
> > >num_cpu)) {
> you also need to check against ITS_CIDBITS limit?
CIDBITS limits is being checked through the s->ct.max_collids member
above
> > +qemu_log_mask(LOG_GUEST_ERROR,
> > +
Re: [PATCH v4 3/8] hw/intc: GICv3 ITS command queue framework
Hi,
On 6/2/21 8:00 PM, Shashi Mallela wrote:
> Added functionality to trigger ITS command queue processing on
> write to CWRITE register and process each command queue entry to
> identify the command type and handle commands like MAPD,MAPC,SYNC.
>
> Signed-off-by: Shashi Mallela
> ---
> hw/intc/arm_gicv3_its.c | 295 +++
> hw/intc/gicv3_internal.h | 37 +
> 2 files changed, 332 insertions(+)
>
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index af60f19c98..6551c577b3 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -49,6 +49,295 @@ static uint64_t baser_base_addr(uint64_t value, uint32_t
> page_sz)
> return result;
> }
>
> +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, bool valid,
> + uint64_t rdbase)
> +{
> +AddressSpace *as = >gicv3->dma_as;
> +uint64_t value;
> +uint64_t l2t_addr;
> +bool valid_l2t;
> +uint32_t l2t_id;
> +uint32_t max_l2_entries;
> +uint64_t cte = 0;
> +MemTxResult res = MEMTX_OK;
> +
> +if (!s->ct.valid) {
> +return res;
> +}
> +
> +if (valid) {
> +/* add mapping entry to collection table */
> +cte = (valid & VALID_MASK) |
> + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL);
> +}
> +
> +/*
> + * The specification defines the format of level 1 entries of a
> + * 2-level table, but the format of level 2 entries and the format
> + * of flat-mapped tables is IMPDEF.
> + */
> +if (s->ct.indirect) {
> +l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE);
> +
> +value = address_space_ldq_le(as,
> + s->ct.base_addr +
> + (l2t_id * L1TABLE_ENTRY_SIZE),
> + MEMTXATTRS_UNSPECIFIED, );
> +
> +if (res != MEMTX_OK) {
> +return res;
> +}
> +
> +valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
> +
> +if (valid_l2t) {
> +max_l2_entries = s->ct.page_sz / s->ct.entry_sz;
> +
> +l2t_addr = value & ((1ULL << 51) - 1);
> +
> +address_space_stq_le(as, l2t_addr +
> + ((icid % max_l2_entries) * GITS_CTE_SIZE),
> + cte, MEMTXATTRS_UNSPECIFIED, );
> +}
> +} else {
> +/* Flat level table */
> +address_space_stq_le(as, s->ct.base_addr + (icid * GITS_CTE_SIZE),
> + cte, MEMTXATTRS_UNSPECIFIED, );
> +}
> +return res;
> +}
Looking at your DevTableDesc and CollTableDesc types again, they are
basically the same except max_devids/max_collids. You may use a single
one and it may be possible to define helpers to access an entry in the
DT or CT.
update_cte/update_dte looks quite similar if you compute the cte and dte
externally and pass a pointer to the associated TableDesc?
Thanks
Eric
> +
> +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset)
> +{
> +AddressSpace *as = >gicv3->dma_as;
> +uint16_t icid;
> +uint64_t rdbase;
> +bool valid;
> +MemTxResult res = MEMTX_OK;
> +uint64_t value;
> +
> +offset += NUM_BYTES_IN_DW;
> +offset += NUM_BYTES_IN_DW;
> +
> +value = address_space_ldq_le(as, s->cq.base_addr + offset,
> + MEMTXATTRS_UNSPECIFIED, );
> +
> +if (res != MEMTX_OK) {
> +return res;
> +}
> +
> +icid = value & ICID_MASK;
> +
> +rdbase = (value >> R_MAPC_RDBASE_SHIFT) & RDBASE_PROCNUM_MASK;
> +
> +valid = (value >> VALID_SHIFT) & VALID_MASK;
> +
> +if ((icid > s->ct.max_collids) || (rdbase > s->gicv3->num_cpu)) {
> +qemu_log_mask(LOG_GUEST_ERROR,
> + "ITS MAPC: invalid collection table attributes "
> + "icid %d rdbase %lu\n", icid, rdbase);
> +/*
> + * in this implementation,in case of error
> + * we ignore this command and move onto the next
> + * command in the queue
> + */
> +} else {
> +res = update_cte(s, icid, valid, rdbase);
> +}
> +
> +return res;
> +}
> +
> +static MemTxResult update_dte(GICv3ITSState *s, uint32_t devid, bool valid,
> + uint8_t size, uint64_t itt_addr)
> +{
> +AddressSpace *as = >gicv3->dma_as;
> +uint64_t value;
> +uint64_t l2t_addr;
> +bool valid_l2t;
> +uint32_t l2t_id;
> +uint32_t max_l2_entries;
> +uint64_t dte = 0;
> +MemTxResult res = MEMTX_OK;
> +
> +if (s->dt.valid) {
> +if (valid) {
> +/* add mapping entry to device table */
> +dte = (valid & VALID_MASK) |
> + ((size & SIZE_MASK) << 1U) |
> + ((itt_addr & ITTADDR_MASK) << 6ULL);
> +}
> +} else {
> +return res;
> +}
> +
> +/*
> + * The specification defines the format of
Re: [PATCH v4 3/8] hw/intc: GICv3 ITS command queue framework
Hi Sashi,
On 6/2/21 8:00 PM, Shashi Mallela wrote:
> Added functionality to trigger ITS command queue processing on
> write to CWRITE register and process each command queue entry to
> identify the command type and handle commands like MAPD,MAPC,SYNC.
>
> Signed-off-by: Shashi Mallela
> ---
> hw/intc/arm_gicv3_its.c | 295 +++
> hw/intc/gicv3_internal.h | 37 +
> 2 files changed, 332 insertions(+)
>
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index af60f19c98..6551c577b3 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -49,6 +49,295 @@ static uint64_t baser_base_addr(uint64_t value, uint32_t
> page_sz)
> return result;
> }
>
> +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, bool valid,
> + uint64_t rdbase)
> +{
> +AddressSpace *as = >gicv3->dma_as;
> +uint64_t value;
> +uint64_t l2t_addr;
> +bool valid_l2t;
> +uint32_t l2t_id;
> +uint32_t max_l2_entries;
> +uint64_t cte = 0;
> +MemTxResult res = MEMTX_OK;
> +
> +if (!s->ct.valid) {
Isn't it a guest log error case. Also you return MEMTX_OK in that case.
Is that what you want?
> +return res;
> +}
> +
> +if (valid) {
> +/* add mapping entry to collection table */
> +cte = (valid & VALID_MASK) |
> + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL);
Do you really need to sanitize rdbase again?
> +}
> +
> +/*
> + * The specification defines the format of level 1 entries of a
> + * 2-level table, but the format of level 2 entries and the format
> + * of flat-mapped tables is IMPDEF.
> + */
> +if (s->ct.indirect) {
> +l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE);
> +
> +value = address_space_ldq_le(as,
> + s->ct.base_addr +
> + (l2t_id * L1TABLE_ENTRY_SIZE),
> + MEMTXATTRS_UNSPECIFIED, );
> +
> +if (res != MEMTX_OK) {
> +return res;
> +}
> +
> +valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
> +
> +if (valid_l2t) {
> +max_l2_entries = s->ct.page_sz / s->ct.entry_sz;
> +
> +l2t_addr = value & ((1ULL << 51) - 1);
> +
> +address_space_stq_le(as, l2t_addr +
> + ((icid % max_l2_entries) * GITS_CTE_SIZE),
> + cte, MEMTXATTRS_UNSPECIFIED, );
> +}
> +} else {
> +/* Flat level table */
> +address_space_stq_le(as, s->ct.base_addr + (icid * GITS_CTE_SIZE),
> + cte, MEMTXATTRS_UNSPECIFIED, );
> +}
> +return res;
> +}
> +
> +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset)
> +{
> +AddressSpace *as = >gicv3->dma_as;
> +uint16_t icid;
> +uint64_t rdbase;
> +bool valid;
> +MemTxResult res = MEMTX_OK;
> +uint64_t value;
> +
> +offset += NUM_BYTES_IN_DW;
> +offset += NUM_BYTES_IN_DW;
May be relevant to add some trace points for debuggability.
> +
> +value = address_space_ldq_le(as, s->cq.base_addr + offset,
> + MEMTXATTRS_UNSPECIFIED, );
> +
> +if (res != MEMTX_OK) {
> +return res;
> +}
> +
> +icid = value & ICID_MASK;
> +
> +rdbase = (value >> R_MAPC_RDBASE_SHIFT) & RDBASE_PROCNUM_MASK;
usually the mask is applied before the shift.
> +
> +valid = (value >> VALID_SHIFT) & VALID_MASK;
use FIELD, see below
> +
> +if ((icid > s->ct.max_collids) || (rdbase > s->gicv3->num_cpu)) {
you also need to check against ITS_CIDBITS limit?
> +qemu_log_mask(LOG_GUEST_ERROR,
> + "ITS MAPC: invalid collection table attributes "
> + "icid %d rdbase %lu\n", icid, rdbase);
> +/*
> + * in this implementation,in case of error
> + * we ignore this command and move onto the next
> + * command in the queue
spec says a command error occurs in that case.
> + */
> +} else {
> +res = update_cte(s, icid, valid, rdbase);
> +}
> +
> +return res;
> +}
> +
> +static MemTxResult update_dte(GICv3ITSState *s, uint32_t devid, bool valid,
> + uint8_t size, uint64_t itt_addr)
> +{
> +AddressSpace *as = >gicv3->dma_as;
> +uint64_t value;
> +uint64_t l2t_addr;
> +bool valid_l2t;
> +uint32_t l2t_id;
> +uint32_t max_l2_entries;
> +uint64_t dte = 0;
> +MemTxResult res = MEMTX_OK;
> +
> +if (s->dt.valid) {
> +if (valid) {
> +/* add mapping entry to device table */
> +dte = (valid & VALID_MASK) |
> + ((size & SIZE_MASK) << 1U) |
> + ((itt_addr & ITTADDR_MASK) << 6ULL);
> +}
> +} else {
> +return res;
> +}
> +
> +/*
> + * The specification defines the format of
Re: [PATCH v4 3/8] hw/intc: GICv3 ITS command queue framework
On Wed, 2 Jun 2021 at 19:00, Shashi Mallela wrote:
>
> Added functionality to trigger ITS command queue processing on
> write to CWRITE register and process each command queue entry to
> identify the command type and handle commands like MAPD,MAPC,SYNC.
>
> Signed-off-by: Shashi Mallela
> ---
> hw/intc/arm_gicv3_its.c | 295 +++
> hw/intc/gicv3_internal.h | 37 +
> 2 files changed, 332 insertions(+)
> +if ((icid > s->ct.max_collids) || (rdbase > s->gicv3->num_cpu)) {
> +qemu_log_mask(LOG_GUEST_ERROR,
> + "ITS MAPC: invalid collection table attributes "
> + "icid %d rdbase %lu\n", icid, rdbase);
> +/*
> + * in this implementation,in case of error
Still missing space after comma.
> + * we ignore this command and move onto the next
> + * command in the queue
> + */
> +} else {
> +res = update_cte(s, icid, valid, rdbase);
> +}
> +
> +return res;
> +}
> +} else {
> +/*
> + * in this implementation,in case of dma read/write error
> + * we stall the command processing
> + */
Ditto.
> +s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, STALLED, 1);
> +qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: %x cmd processing failed!!\n", __func__, cmd);
The double-exclamation marks are unnecessary :-)
> +break;
> +}
> +}
> +}
Otherwise
Reviewed-by: Peter Maydell
thanks
-- PMM
