On Tue, Jun 18, 2024 at 02:51:41PM -0700, Nicolin Chen wrote:
> On Tue, Jun 18, 2024 at 05:34:21PM -0400, Michael S. Tsirkin wrote:
> > On Tue, Jun 18, 2024 at 02:19:25PM -0700, Nicolin Chen wrote:
> > > On Tue, Jun 18, 2024 at 05:14:32PM -0400, Michael S. Tsirkin wrote:
> > > > > @@ -306,8 +314,8 @@ build_iort(GArray *table_data, BIOSLinker
> > > > > *linker, VirtMachineState *vms)
> > > > > }
> > > > >
> > > > > /* Append the last RC -> ITS ID mapping */
> > > > > - if (next_range.input_base < 0xFFFF) {
> > > > > - next_range.id_count = 0xFFFF - next_range.input_base;
> > > > > + if (next_range.input_base < 0x10000) {
> > > > > + next_range.id_count = 0x10000 - next_range.input_base;
> > > > > g_array_append_val(its_idmaps, next_range);
> > > > > }
> > > >
> > > > A change of logic here - I think the new one is right and old
> > > > one was wrong, actually. Right?
> > >
> > > Sorry, I don't quite follow that question...
> > >
> > > Doesn't a patch correct an old wrong one to a new right one?
> > >
> > > Thanks
> > > Nicolin
> >
> >
> > So if base is 0xFFFF what should happen? I think previously we
> > skipped an entry and that is wrong. So that's another latent
> > bug this patch fixes then?
> > Worth documenting in the commit log too.
>
> I had noticed that -- yes, ideally it should have been "<= 0xFFFF".
> Yet, practically input_base can never be 0xFFFF as it's calculated:
>
> hw/arm/virt-acpi-build.c:245: .input_base = min_bus << 8,
> hw/arm/virt-acpi-build.c:305: next_range.input_base =
> idmap->input_base + idmap->id_count;
>
> The first one always sets input_base to 0xXX00 (min_bus = 0xXX).
> The second one, as we know for id_count, must be 0xZZ00 too since
> input_base from the first place must be 0xXX00 and 0xYY00
> hw/arm/virt-acpi-build.c:301: next_range.id_count =
> idmap->input_base - next_range.input_base;
>
> So, it's a case that could never be triggered? Probably not worth
> highlighting IMOH...
>
> Thanks
> Nicolin
Makes sense.
