On 22.11.2023 01:37, Andrew Cooper wrote:
> On 16/11/2023 1:47 pm, Jan Beulich wrote:
>> @@ -427,6 +428,47 @@ static int cf_check pit_save(struct vcpu
>> return rc;
>> }
>>
>> +static int cf_check pit_check(const struct domain *d, hvm_domain_context_t
>> *h)
>> +{
>> + const struct hvm_hw_pit *hw;
>> + unsigned int i;
>> +
>> + if ( !has_vpit(d) )
>> + return -ENODEV;
>> +
>> + hw = hvm_point_entry(PIT, h);
>> + if ( !hw )
>> + return -ENODATA;
>> +
>> + /*
>> + * Check to-be-loaded values are within valid range, for them to
>> represent
>> + * actually reachable state. Uses of some of the values elsewhere
>> assume
>> + * this is the case. Note that the channels' mode fields aren't
>> checked;
>> + * older Xen might save them as 0xff.
>
> "older Xen" goes stale very quickly. "Xen prior to 4.19", or "Xen prior
> to the time of writing (Nov 2023)" if you're planning to backport this.
Can certainly adjust. And no, I don't think I intend to backport this.
>> @@ -443,6 +485,14 @@ static int cf_check pit_load(struct doma
>> goto out;
>> }
>>
>> + for ( i = 0; i < ARRAY_SIZE(pit->hw.channels); ++i )
>> + {
>> + struct hvm_hw_pit_channel *ch = &pit->hw.channels[i];
>> +
>> + if ( (ch->mode &= 7) > 5 )
>> + ch->mode -= 4;
>
> How does this work? If we get in an 0xff, we'll turn it into 0xfb
> rather than 7.
Did you overlook the &= ?
>> @@ -575,7 +625,7 @@ void pit_reset(struct domain *d)
>> for ( i = 0; i < 3; i++ )
>> {
>> s = &pit->hw.channels[i];
>> - s->mode = 0xff; /* the init mode */
>> + s->mode = 7; /* the init mode */
>
> I think it would be helpful to modify the comment to say /* unreachable
> sentinel */ or something.
Can change, sure.
Jan
